Error out if alias tensor args are consumed by in-place ops#798
Merged
Conversation
t-vi
reviewed
Jul 18, 2024
t-vi
requested changes
Jul 18, 2024
Collaborator
t-vi
left a comment
t-vi
left a comment
There was a problem hiding this comment.
Hi @crcrpar , thank you for working on this!
I am not sure that we're checking at the right place - and that we should be putting this on the TensorProxies:
- to my mind, we need to do the aliasing check on every invocation (including cached), with all the speed requirement implications, so maybe only for tensors where it is relevant...
- if we wanted to support this down the road (shared weights might be an important case depending on whether they come out as different or same args), we would need to check for this in the prologue (but this is future probably).
crcrpar
force-pushed
the
crpa/alias-detector
branch
4 times, most recently
from
ee000e7 to
8351339
Compare
crcrpar
changed the title
crcrpar
force-pushed
the
crpa/alias-detector
branch
from
1e53c01 to
98b3fab
Compare
crcrpar
force-pushed
the
crpa/alias-detector
branch
from
98b3fab to
fb00905
Compare
crcrpar
force-pushed
the
crpa/alias-detector
branch
from
ad6e536 to
5a286f7
Compare
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
crcrpar
force-pushed
the
crpa/alias-detector
branch
from
5a286f7 to
52c7291
Compare
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
crcrpar
force-pushed
the
crpa/alias-detector
branch
from
52c7291 to
ac1481b
Compare
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
t-vi
reviewed
Aug 1, 2024
| # alaises wouldn't matter, thus it'd be better to nullify this entry in such cases. | ||
| # It however would require the functionalized computation trace to interact with `cache_info`, | ||
| # which seems to break the consistency of cache_info, leading to a failure in cache_info check. | ||
| cache_info["alias_tensor_indices"] = _alias_tensor_of_args_kwargs(*args, **kwargs) |
Collaborator
There was a problem hiding this comment.
While you would not want to change the cache info, you can grab the prologue check for it and remove it. It would be better to actually not collect the information in this case, but I guess that's how it is for now.
Collaborator
Author
There was a problem hiding this comment.
If we were to support the combo of in-place ops and alias tensor args, I do find it better to have separate traces whether or not the args include alias tensors
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before submitting
What does this PR do?
Fixes # (issue).
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃